Skip to content

Conversation

@ermyar
Copy link
Collaborator

@ermyar ermyar commented Oct 24, 2025

Revised error using, added new type CodeErrors to compare them using errors.Is(). Also used to check that request's ctx was cancelled. Also updated state CodeErrors.

Fixes (#457)

I didn't forget about (remove if it is not applicable):

Related issues: (#457)

bigbes and others added 3 commits October 16, 2025 14:08
* Module version update from v2 to v3,
* Go version dep updated from 1.20 to 1.24,
* Added stub record for migration guide.
Add support for `go-option` to the `arrow`, `datetime`, `decimal`, and `uuid`
packages. This allows for handling optional values of the `Arrow`, `Datetime`,
`Interval`, `Decimal`, `UUID` and `BoxError` types.

The following changes have been made:
- Added `go:generate` directives to the `Arrow`, `Datetime`, `Interval`,
  `Decimal`, `UUID` and `BoxError` types to generate optional types using
  `github.com/tarantool/go-option/cmd/gentypes`.
- Implemented `MarshalMsgpack` and `UnmarshalMsgpack` methods for the `Arrow`,
  `Datetime`, `Interval`, and `Decimal` types.
- Added `marshalUUID` and `unmarshalUUID` functions for the `UUID` type.
- The generated `_gen.go` files contain the `Optional*` types that wrap the
  original types and provide methods to handle optional values, including
  `EncodeMsgpack` and `DecodeMsgpack` for `msgpack` serialization.
- Refactored the `datetime` and `decimal` decoders to use the new
  `UnmarshalMsgpack` methods.
@ermyar ermyar marked this pull request as ready for review October 24, 2025 11:02
@ermyar ermyar requested a review from oleg-jukovec October 24, 2025 12:01
@oleg-jukovec oleg-jukovec requested a review from bigbes October 24, 2025 12:13
@ermyar ermyar linked an issue Oct 24, 2025 that may be closed by this pull request
@ermyar ermyar force-pushed the ermyar/gh-457_cancelled_ctx_error branch from 35999b5 to 99a30a9 Compare October 24, 2025 14:12
Revised error use, added new type CodeErrors to compare them using
errors.Is(). Also used to check that request's ctx was cancelled.

Fixes (#457)
@ermyar ermyar force-pushed the ermyar/gh-457_cancelled_ctx_error branch from 99a30a9 to 6b1bbe6 Compare October 24, 2025 23:09
@oleg-jukovec oleg-jukovec changed the title fix: returning comparable error on contex's cancelled errorcase. fix: returning comparable error on contex's cancelled errorcase Oct 27, 2025
@oleg-jukovec
Copy link
Collaborator

Please, change the base branch to master.

Copy link
Collaborator

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, add support of ability to unwrap/compare an original context error instead of the code.

We should have an entry in the CHANGELOG.md for the change too.

// Output:
// Ping Resp data []
// Ping Error context is done (request ID N)
// Ping Error context is done (request ID N) (NxN)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The format above should replace only the request ID, please fix the replace pattern.

Suggested change
// Ping Error context is done (request ID N) (NxN)
// Ping Error context is done (request ID N) (0x4008)

if !contextDoneErrRegexp.Match([]byte(err.Error())) {
t.Fatalf("Failed to catch an error from done context")
}
// checking that we could use errors.Is to get known about error.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// checking that we could use errors.Is to get known about error.
// Checking that we could use errors.Is to get known about error.

t.Fatalf("Failed to catch an error from done context")
}
// checking that we could use errors.Is to get known about error.
require.True(t, errors.Is(err, ErrCancelledCtx))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem of the approach - we unable to get an original context error here. But we should.

Suggested change
require.True(t, errors.Is(err, ErrCancelledCtx))
require.True(t, errors.Is(err, ErrCancelledCtx))

@ermyar ermyar changed the base branch from v3 to master October 27, 2025 13:26
@ermyar ermyar closed this Oct 27, 2025
@ermyar ermyar reopened this Oct 27, 2025
@ermyar ermyar closed this Oct 27, 2025
@ermyar ermyar deleted the ermyar/gh-457_cancelled_ctx_error branch October 27, 2025 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

v3: return context error after it is done

4 participants